Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds CPU profiling capability to the debug bundle by implementing RPC methods to start and stop profiling in the daemon, capturing the profile data during debug bundle collection.
- Adds StartCPUProfile and StopCPUProfile RPC methods to the daemon service
- Integrates CPU profiling into the debug bundle generation workflow
- Captures profiling data during the debug bundle collection window
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| client/proto/daemon.proto | Defines new RPC methods and message types for CPU profiling |
| client/proto/daemon.pb.go | Generated protobuf code for CPU profiling messages |
| client/proto/daemon_grpc.pb.go | Generated gRPC client/server code for CPU profiling methods |
| client/server/server.go | Adds fields to track CPU profiling state and buffer |
| client/server/debug.go | Implements StartCPUProfile and StopCPUProfile RPC handlers |
| client/internal/debug/debug.go | Integrates CPU profile data into debug bundle archive |
| client/ui/debug.go | Starts CPU profiling before collecting debug data |
| client/cmd/debug.go | Adds CPU profiling lifecycle management to CLI debug command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds CPU profiling to the debug workflow: new StartCPUProfile/StopCPUProfile RPCs, server-side profiling state and pprof handling, CLI/UI lifecycle integration to start/stop profiling, and bundling of captured cpu.prof into debug archives via a new GeneratorDependencies field. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI/CLI
participant Server as Server
participant Profiler as pprof
participant Bundle as Debug Bundle
UI->>Server: StartCPUProfile()
activate Server
Server->>Server: lock mutex\nif profiling -> return error\ninit buffer, cpuProfiling=true
Server->>Profiler: pprof.StartCPUProfile(buffer)
Server-->>UI: StartCPUProfileResponse
deactivate Server
Note over Server,Profiler: CPU profiling runs, data written to buffer
UI->>UI: collect debug data / runForDuration elapses
UI->>Server: StopCPUProfile()
activate Server
Server->>Server: lock mutex\nif not profiling -> return error\npprof.StopCPUProfile(), cpuProfiling=false
Server-->>UI: StopCPUProfileResponse
deactivate Server
UI->>Server: Request DebugBundle()
activate Server
Server->>Bundle: Provide DebugBundle request (includes CPUProfile bytes)
Bundle->>Bundle: addCPUProfile() -> write cpu.prof into archive
Bundle-->>UI: debug bundle (with cpu.prof)
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
client/cmd/debug.go(3 hunks)client/internal/debug/debug.go(6 hunks)client/proto/daemon.proto(2 hunks)client/server/debug.go(4 hunks)client/server/server.go(2 hunks)client/ui/debug.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/ui/debug.go (1)
client/proto/daemon.pb.go (6)
StartCPUProfileRequest(5230-5234)StartCPUProfileRequest(5247-5247)StartCPUProfileRequest(5262-5264)StopCPUProfileRequest(5304-5308)StopCPUProfileRequest(5321-5321)StopCPUProfileRequest(5336-5338)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: JS / Lint
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
🔇 Additional comments (12)
client/ui/debug.go (2)
412-414: LGTM - CPU profiling integration is appropriately non-blocking.The error is logged as a warning and the debug flow continues, which is correct since CPU profiling is an optional enhancement to the debug bundle.
457-459: LGTM - StopCPUProfile placement is correct.Stopping CPU profiling after the duration wait and before collecting final status ensures the profile captures the entire debug collection period.
client/internal/debug/debug.go (2)
533-544: LGTM - Clean implementation following existing patterns.The
addCPUProfilemethod correctly:
- Returns early when no CPU profile data is available
- Uses the same
addFileToZippattern as other profile methods- Provides appropriate error wrapping
334-336: LGTM - CPU profile addition integrates well with existing archive creation flow.The CPU profile is added after the standard runtime profiles, with error logging that doesn't interrupt bundle generation.
client/server/server.go (1)
85-87: LGTM - CPU profiling state fields properly added to Server struct.The fields are appropriately scoped within the Server struct and will be protected by the existing mutex as shown in the debug.go implementation.
client/server/debug.go (3)
37-43: LGTM - Safe CPU profile data extraction with proper cleanup.The deferred cleanup correctly nils the buffer reference after bundle generation completes. Since
Bytes()returns a slice referencing the buffer's internal array, and the cleanup only nils the pointer (not the underlying data), this is safe.
230-249: LGTM - StartCPUProfile implementation is thread-safe.Good implementation:
- Sets
cpuProfiling = truebefore callingpprof.StartCPUProfile(addressing the previous review feedback)- Proper cleanup on error (resets both buffer and flag)
- Mutex protection throughout
252-268: LGTM - StopCPUProfile correctly preserves the buffer for later retrieval.The implementation properly stops profiling and resets the flag while preserving the buffer data for
DebugBundleto consume later.client/proto/daemon.proto (2)
96-101: LGTM - RPC definitions follow existing conventions.The new CPU profiling RPCs are well-documented and follow the established patterns in the service definition.
764-775: LGTM - Message definitions are appropriately minimal.Empty request/response messages are the right choice for these simple control operations where no parameters or return data are needed beyond success/failure.
client/cmd/debug.go (2)
223-235: Well-designed cleanup pattern for CPU profiling.The defer ensures profiling is stopped in case of early return (e.g., if
waitForDurationOrCancelfails at line 241), while the flag-based guard prevents attempting to stop profiling that never started or was already stopped. This is solid defensive programming.Note: The explicit stop at lines 245-250 is intentional and necessary—it must execute before the bundle is created so the profile data is finalized and available to the
DebugBundleRPC.
403-403: Correct: CPU profiling not applicable in SIGUSR1 flow.Setting
CPUProfile: nilis appropriate here. ThegenerateDebugBundlefunction handles signal-based (SIGUSR1) debug bundle generation without CPU profiling, whereas therunForDurationflow uses the RPC-basedDebugBundlecall where the daemon server manages CPU profile inclusion.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/server/debug.go`:
- Around line 29-35: The DebugBundle code currently reads
s.cpuProfileBuf.Bytes() which can race with pprof background writes; update
DebugBundle to check the cpuProfiling boolean flag (the same flag set by
StartCPUProfile/StopCPUProfile) and skip capturing/including cpuProfileData when
cpuProfiling is true (i.e., profiling still active), only reading
s.cpuProfileBuf.Bytes() when cpuProfiling is false; keep the existing deferred
s.cpuProfileBuf = nil behavior when you do capture, and ensure you reference
s.cpuProfileBuf, cpuProfiling, DebugBundle, StartCPUProfile and StopCPUProfile
to locate and fix the code paths.
♻️ Duplicate comments (1)
client/cmd/debug.go (1)
241-247: Remove the redundant StopCPUProfile call.
There’s both a deferred stop and an explicit stop; keep one to avoid double-stop.
|
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.